Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/sbor encoder decoder traits #597

Merged
merged 21 commits into from
Nov 24, 2022

Conversation

dhedey
Copy link
Contributor

@dhedey dhedey commented Nov 19, 2022

As per discussions with Yulong, this PR implements changes to the Encode/Decode and Encoder/Decoder traits, including now returning an EncodeError.

There needs to be symmetry between Encode and Decode to ensure that there aren't any attack differentials where (eg) we save a substate but can't read it back... I'd appreciate if during code review we have another pair of eyes on this.

Some Asides

I originally set the max SBOR recursion depth to 32, but this choked some tests (where they failed to store a Package substate due to the recursion in some of the ABIs) as they needed a depth of 34 to pass. For now, I've gone with 64 - which seems much less likely to choke in non-malicious situations. When we linearise schemas, the ABI depth won't be a problem.

With the new encoder, I was also hitting WASM native stack depth limits during creation of the ABI when the ABI contained a deeply nested structure such as ComponentInfoSubstate containing AuthRules. My solution has been to raise stack depths from 512 to 1024 across the board, as if we can hit them in normal use case, it feels a bit wrong. Does this feel okay? (This was incredibly hard to debug because it just returned a WASM panic... Heads up for anyone hitting weird panics in future. Also note that the

In terms of WASM size for (eg) the faucet.wasm

  • After adding the Decoder trait, the size increased from 173KB to 178 KB
  • After adding the Encoder trait and EncodeError, the size increased up to 225KB
  • Hackily replacing EncodeError with panics and an "infallible" empty enum for testing purposes (to avoid all the branching logic) brings it back down to 202KB.

I'm not really sure why the code has grown by so much, even with the infallible branches removed - Yulong, might be interesting to take a look at the WASM to see if I've made a mistake and included code accidentally or something 🤷‍♂️

Still to come in another PR:

  • A test that we get a sane error if a component struct tries to save too a struct which is too deep to encode/decode

Breaking Changes:

  • SBOR - The Decode<X> trait has changed to Decode<X: CustomTypeId, D: Decoder<X>> to allow swapping out the decoder (eg to support other decoder implementations in future - eg streaming decoding)
  • SBOR - The Encode<X> trait has changed to Encode<X: CustomTypeId, E: Encoder<X>> to allow swapping out the encoder (eg to support other encoder implementations in future - eg streaming encoding; or throwing errors if too much space is used)
  • SBOR - There is now a max recursive depth of 64 during SBOR decoding and encoding
  • SBOR - The Encode trait's methods now return Result<(), EncodeError>, with encode errors for exceeding the 32 depth or for the max size being exceeded.
  • SBOR - encode_any, encode_any_with_buffer and decode_any have been removed - instead, you can simply encode/decode an SborValue or scrypto_encode/scrypto_decode a ScryptoValue
  • Scrypto SBOR - the traits ScryptoEncode, ScryptoDecode and ScryptoTypeId have been introduced. These can be thought of as "aliases" which should be used for arguments, and have a blanket implementation from the fundamental Encode/Decode/TypeId traits. These fundamental traits should be used for any implementations.
  • Scrypto SBOR - scrypto_encode (and similar functions) now return Result<Vec<u8>, EncodeError>

@dhedey dhedey mentioned this pull request Nov 19, 2022
@dhedey dhedey marked this pull request as ready for review November 20, 2022 15:31
Copy link
Member

@iamyulong iamyulong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good to me.

The wasm size increase is a bit concerning - primarily due to extra logic to the deal with result and unwrap.

The error propagation is nice but I wonder if it makes more sense to make a comprise by treating SizeTooLarge and MaxDepthExceeded invariants as majority of the clients are just unwrapping/expecting anyway. Panics!

EncodeError propagation is usefull when the system creates/transforms/encloses "any" value based on user inputs, and attempts to encode.

Not a blocker.

radix-engine-interface/src/data/mod.rs Outdated Show resolved Hide resolved
radix-engine-interface/src/data/mod.rs Show resolved Hide resolved
sbor/src/basic.rs Show resolved Hide resolved
sbor/src/basic.rs Show resolved Hide resolved
sbor/src/value.rs Outdated Show resolved Hide resolved
sbor/src/codec/result.rs Show resolved Hide resolved
sbor/src/codec/tuple.rs Show resolved Hide resolved
Copy link
Member

@iamyulong iamyulong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@dhedey dhedey merged commit 56bd796 into develop Nov 24, 2022
@talekhinezh talekhinezh deleted the feature/sbor-encoder-decoder-traits branch February 21, 2023 15:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants